Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jelte's not-yet-deployed commits #7

Open
wants to merge 12 commits into
base: prod-mirror
Choose a base branch
from
Open

Conversation

JelteF
Copy link
Collaborator

@JelteF JelteF commented Jan 22, 2025

This PR contains bunch of commits that have been running on the staging environment for a while now, but haven't been deployed yet. Reviews of any of them would be highly appreciated. The commit messages of each patch have details on the reasoning. If you want to access the staging environment, there's currently a password on it, but I can provide it to you if you want to test the changes.

  • CFbot integration: Has already been through multiple rounds of review from Magnus, and should now be ready to deploy.
  • Integrate previous CFbot links with new code
  • Make CFbot add a history item when patch needs rebase
  • Add a basic editorconfig file
  • Add ID column to commitfest page
  • Make sorting a toggle
  • Allow sorting by name
  • Allow sorting using the header of the closed patches too
  • Update readme with correct django version
  • Make /patch/{id} the URL for a patch: This one could use some additional testing, because old links in the code should be updated to use the new URLs.
  • Adjust direction of dropdowns at the bottom of the page (Adjust direction of dropdowns at the bottom of the page #4): Not by me, but has been reviewed/approved/tested by me

JelteF and others added 11 commits January 12, 2025 21:42
This adds our own custom replication between the CFbot and the
commitfest app. We currently keep the last CFbot runs in our database,
in an attempt to keep the queries easy and efficient. The CFbot results
are now shown on the overview page of each commitfest, as well as on the
page for each specific patch.
In previous commits (2ada851 & b6010e9) some initial links and git
checkout instructions were added to the patch page. I received some in
person feedback at PgConf EU that those used quite some vertical space
on the page.

Now that we have a dedicated row in the table for CFbot statuses, it
seems natural to integrate those previous links and checkout
instructions into that row. Both to keep all CFbot content together, as
well as to reduce vertical space needed.

To reduce the necessary space needed for the checkout instructions,
these previously explicit checkout instructions have been changed to
button that copies the required commands to the clipboard.

Finally, this also makes "Needs rebase!" link to CFbot logs instead of
CI results. I updated these logs recently on the CFbot side to become
much more useful to look at, by making them actually show the conflicts.
This is needed to send notifications to a patch author that it
subscribed. We don't send updates to other subscribers to the patch, as
this information is generally only useful for the author themselves.

By using the "history item" infrastructure it's also displayed on the
patch page in the "History" table. That seems quite useful too, because
it makes it clear how long the patch has needed a rebase.
This is mostly useful to avoid people from adding trailing whitespace to
files.
The ID of a CF entry is the only stable identifier (people can change
the name). That's why tooling such as CFbot uses the ID of the patch for
a lot of things (including showing it on the cfbot overview page).
Having it visible on the page is quite useful for debugging purposes

In eee60a5 the ID was added to the title of the entry, but that made the
the title of the page itself less prominent. So this is an attempt to
have the information available, but not too prominently visible.
If the commitfest entries are sorted by a column clicking the header
again will now remove the sort. In a future commit, it would be nice to
also support for reverse sorting.
It was pretty confusing that clicking the patch name header would sort
by created time, grouped by topic. This makes that sort by name.
Sorting order impacts closed patches too. So not showing the arrow that
indicates sort direction on that header is confusing. While we're at it,
we might as well allow people to toggle sorting direction using that
header too.

This also automatically fixes the problem that the cell in the closed
patches header did not contain any title at all.
Previously we'd include the ID of the commitfest in the URL of the
patch. In 9f12a5e we introduced a stable URL for patches that would
redirect to the one for the latest commitfest. This starts to use that
URL as the valid only URL for a patch (with the previous URL redirecting
to this one).

The reasoning behind this is that the old approach resulted in N
different URLs for each patch, which all showed the exact same patch
information. The only difference between all these URLs would be the
breadcrumb at the top of the page.

The only benefit of that approach is that if you're on an old
commitfest, and click a link there, then the breadcrumb will bring you
back to where you came from. Since people rarely have a reason to browse
closed commitfests, the that benefit seems pretty small. Especially
because people can just as well press their browser back button, in that
case.

The problems that these N links cause seem much more impactful to most
users:

1. If you click an old link to a cf entry (e.g. one from an email in the
   archives), then the breadcrumb will contain some arbitrarily old
   commitfest. It seems much more useful to have the breadcrumb show the
   commitfest that the patch is currently active in (or got
   committed/rejected in).
2. If you arrive on such an old link you also won't be able to change
   the status. Instead you'd get a message like: "The status of this
   patch cannot be changed in this commitfest. You must modify it in the
   one where it's open!". Which requires you to go to the latest page.
3. Places that use the stable URLs require an extra round-trip to
   actually get to the patch page.
4. It's a bit confusing that old pages of a patch still get updated with
   all the new information, i.e. why have all these pages if they
   contain the exact same content.
5. Problem 3 is generally also bad for Search Engine Optimization (SEO),
   for now we don't care much about that though.

Finally this also changes the links on the patch page itself for each of
the commitfests that a patch has been part of. Those links were already
rather useless, since all they effectively did was change the
breadcrumb. But with this new commit, they wouldn't even do that anymore,
and simply redirect to the current page. So now they start pointing to
the commitfest itself, which seems more useful behaviour anyway.

Co-Authored-By: Jacob Brazeal <[email protected]>
The bottom dropdowns on the patch page would expand downwards, requiring
the user to scroll down to see and click any of the buttons in the dropdown.
With this change these are changed into "dropup" menus, so the expand upwards.
@JelteF JelteF changed the title Jelte's inflight patches Jelte's not-yet-deployed patches Jan 22, 2025
@JelteF JelteF changed the title Jelte's not-yet-deployed patches Jelte's not-yet-deployed commits Jan 22, 2025
This PR implements a way to use dummy data for development in two ways:

1. Dump a working dev database with Django's `dumpdata` command. We dump
the `auth` and `commitfest` modules separately. This data can likewise
be reloaded when starting from scratch with the corresponding `loaddata`
commands (see the README.)
2. Mocks the archives server, to allow users to search and add sample
mailing threads to their patches.

To avoid an infinite recursion error this change also required moving
the ManyToMany relationship between MailThread and Patch from the
MailThread to Patch side.

---------

Co-authored-by: Jelte Fennema-Nio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants